Clean up some tests in tests/ui #138471

spencer3035 · 2025-03-14T00:23:16Z

I cleaned up 3 top level tests, keeping the changes minor because it is my first PR and wanted to get feedback before doing more changes/PRs.

Tracking issues:
#73494
#133895

r? jieyouxu

jieyouxu

Thanks for the PR! Always glad to see test cleanup/improvement efforts.

I left a lot of feedback on individual test changes since you were looking for feedback (to maybe guide your future test changes), and some of them are way more nitpicky than I may fixate on normally.

I reviewed your changes commit-by-commit, so my review comments are best viewed commit-by-commit too.

I'll also point out a commit history nit here: to make it easier during review, I would structure the commits like:

Commit 1 renames test X (maybe move directory and/or rename).
Commit 2 fixes/documents test X.

Notably, I would rebase and fixup test stdout/stderr reblesses to the second "fixes/documents" commit. Then when we decide together after iterations that the test changes look good, I'll ask you to adjust the commit history to have 1 commit per individual test (this is to help future git archaeology).

BTW, you can also add in your PR descriptions that your PR is "Part of #133895" efforts 😁

tests/ui/modules/mod-cfg-multiple-targets.rs

tests/ui/duplicate_entry_error.rs

tests/ui/modules/mod-cfg-multiple-targets.rs

tests/ui/error-codes/E0381-duplicated-label.rs

tests/ui/duplicate_entry_error.rs

spencer3035 · 2025-03-14T23:39:36Z

I tried to keep the history linear for now. ~~In the end, we want to rebase to one commit per test correct?~~

Edit: I read your comment above that answers this question.

jieyouxu · 2025-03-15T08:24:34Z

Cool. BTW you can mark your PR as ready for review via @rustbot ready (see https://rustc-dev-guide.rust-lang.org/contributing.html#waiting-for-reviews), otherwise I use the S-waiting-on-xx to filter out PRs in my review queue that needs action from me (i.e. review).

jieyouxu

Thanks! The changes themselves look good, I left a few comment nits but no real functional changes.

At this point in the review, I'm fairly confident that the changes are mostly ready to merge to master, so I am requesting that you adjust the commit history to have 1 commit for each test, e.g. sth like

Commit 1 improves outer-mod-attr-... test
Commit 2 improves duplicate lang item test
Commit 3 improves duplicate label test

To help future git archaeology. If these tests were actually (closely) related then I would've requested just to squish them into one commit, but that's not the case here, which is why I am requesting one commit per different test.

tests/ui/attributes/outer-mod-attr-applies-only-to-first.rs

tests/ui/duplicate_entry_error.rs

tests/ui/error-codes/E0152-duplicate-lang-items.rs

jieyouxu · 2025-03-15T08:47:35Z

(I resolved all review comments from the previous review pass because I found them (more than) adequately addressed, but you may wish to expand a few of them since I also left some extra info/feedback. GitHub likes to hide the resolved comments so just wanted to let you know.)

spencer3035 · 2025-03-15T17:04:31Z

I addressed the comments and rebased to one commit per test. This process has made me way better at modifying git history and organizing commits.

I'm assuming the CI will pass because it passed locally.

@rustbot ready

jieyouxu

Looks good, thanks!

jieyouxu · 2025-03-16T01:36:22Z

I'm assuming the CI will pass because it passed locally.

Not necessarily the case, because CI could have some environmental differences (e.g. platforms) versus local environments. But the tests here are mostly target/env-agnostic so they should be fine.

Anyway,

@bors r+ rollup

bors · 2025-03-16T01:36:25Z

📌 Commit 27077b9 has been approved by jieyouxu

It is now in the queue for this repository.

Rollup of 16 pull requests Successful merges: - rust-lang#133055 (Expand `CloneToUninit` documentation.) - rust-lang#137147 (Add exclude to config.toml) - rust-lang#137864 (Don't drop `Rvalue::WrapUnsafeBinder` during GVN) - rust-lang#137890 (doc: clarify that consume can be called after BufReader::peek) - rust-lang#137956 (Add RTN support to rustdoc) - rust-lang#137968 (Properly escape regexes in Python scripts) - rust-lang#138082 (Remove `#[cfg(not(test))]` gates in `core`) - rust-lang#138275 (expose `is_s390x_feature_detected!` from `std::arch`) - rust-lang#138303 (Fix Ptr inconsistency in {Rc,Arc}) - rust-lang#138309 (Add missing doc for intrinsic (Fix PR135334)) - rust-lang#138323 (Expand and organize `offset_of!` documentation.) - rust-lang#138329 (debug-assert that the size_hint is well-formed in `collect`) - rust-lang#138465 (linkchecker: bump html5ever) - rust-lang#138471 (Clean up some tests in tests/ui) - rust-lang#138472 (Add codegen test for rust-lang#129795) - rust-lang#138484 (Use lit span when suggesting suffix lit cast) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#138471 - spencer3035:move-ui-test-1ofn, r=jieyouxu Clean up some tests in tests/ui I cleaned up 3 top level tests, keeping the changes minor because it is my first PR and wanted to get feedback before doing more changes/PRs. Tracking issues: rust-lang#73494 rust-lang#133895 r? jieyouxu

rustbot assigned jieyouxu Mar 14, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 14, 2025

This comment has been minimized.

Sign in to view

jieyouxu requested changes Mar 14, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2025

jieyouxu mentioned this pull request Mar 14, 2025

Tracking Issue for ui test suite improvements #133895

Open

This comment has been minimized.

Sign in to view

jieyouxu approved these changes Mar 15, 2025

View reviewed changes

spencer3035 added 3 commits March 15, 2025 10:52

improves outer mod attribute test

9f06585

improves duplicate label test

6207559

improves duplicate lang item test

27077b9

spencer3035 force-pushed the move-ui-test-1ofn branch from 52ed423 to 27077b9 Compare March 15, 2025 16:56

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 15, 2025

jieyouxu approved these changes Mar 16, 2025

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 16, 2025

jieyouxu mentioned this pull request Mar 16, 2025

Rollup of 16 pull requests #138548

Merged

bors merged commit 7c1555c into rust-lang:master Mar 16, 2025
6 checks passed

rustbot added this to the 1.87.0 milestone Mar 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up some tests in tests/ui #138471

Clean up some tests in tests/ui #138471

spencer3035 commented Mar 14, 2025 •

edited

Loading

This comment has been minimized.

jieyouxu left a comment •

edited

Loading

spencer3035 commented Mar 14, 2025 •

edited

Loading

This comment has been minimized.

jieyouxu commented Mar 15, 2025

jieyouxu left a comment

jieyouxu commented Mar 15, 2025 •

edited

Loading

spencer3035 commented Mar 15, 2025

jieyouxu left a comment

jieyouxu commented Mar 16, 2025

bors commented Mar 16, 2025

Clean up some tests in tests/ui #138471

Clean up some tests in tests/ui #138471

Conversation

spencer3035 commented Mar 14, 2025 • edited Loading

This comment has been minimized.

jieyouxu left a comment • edited Loading

Choose a reason for hiding this comment

spencer3035 commented Mar 14, 2025 • edited Loading

This comment has been minimized.

jieyouxu commented Mar 15, 2025

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu commented Mar 15, 2025 • edited Loading

spencer3035 commented Mar 15, 2025

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu commented Mar 16, 2025

bors commented Mar 16, 2025

spencer3035 commented Mar 14, 2025 •

edited

Loading

jieyouxu left a comment •

edited

Loading

spencer3035 commented Mar 14, 2025 •

edited

Loading

jieyouxu commented Mar 15, 2025 •

edited

Loading